Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add prompt_pin and send_pin HWI commands #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ben-kaufman
Copy link

@ben-kaufman ben-kaufman commented Sep 3, 2024

Add support for HWI prompt_pin and send_pin commands (for Trezor One).

Not sure there's a good way to automate testing for that with an emulator though.

Tested on a physical Trezor One.

src/lib.rs Outdated Show resolved Hide resolved
@notmandatory
Copy link
Member

Concept ACK. Please add a comment if you were able to manually test this with a physical device.

@ben-kaufman
Copy link
Author

Thanks @notmandatory, I've added an ignore note.

I have also added the prompt PIN command.

Also something important to note, in the test, I had to use get_client and specify device path due to this bug in HWI itself: bitcoin-core/HWI#636

The problem is that currently, due to how we process errors in enumerate, it will not be possible to get the path of the Trezor One using Rust HWI, which makes this a lot less useful. This is because when the enumerate encounters an error, it won't return the rest of the device data like its path, but instead will only return an HWI Error. This prevents us from learning the path, and being able to use it in get client before the device is unlocked.

So what can be done is to instead of returning an error, return the fields we can, and add an error field to HWIDevice itself. If that solution makes sense let me know and I could implement it. It will require some chnages to the data structures though, which is why I want to know first if this would be a good way to do it.

@ben-kaufman ben-kaufman changed the title Add send_pin HWI command Add prompt_pin and send_pin HWI command Sep 8, 2024
@ben-kaufman ben-kaufman changed the title Add prompt_pin and send_pin HWI command Add prompt_pin and send_pin HWI commands Sep 8, 2024
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 1bdbcbc

@notmandatory notmandatory mentioned this pull request Sep 23, 2024
@notmandatory
Copy link
Member

@ben-kaufman do you want to get #105 in before we do a new release for this one?

@ben-kaufman
Copy link
Author

@notmandatory sure, I think I can resolve the conflicts this week. It's mainly just getting the new signer component to use the new structure.

@ben-kaufman
Copy link
Author

@notmandatory I believe after merging this, we should also handle the issue that prevents an effective use of it by changing how device errors are handled on enumerate. Is there a good place to discuss the best way to handle these errors, or should I just submit a suggested implementation PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants